Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Menu fixes #7017

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Menu fixes #7017

wants to merge 3 commits into from

Conversation

edleeks87
Copy link
Contributor

@edleeks87 edleeks87 commented Oct 14, 2024

fix #6934, fix #7000, fix #7010

Proposed behaviour

Ensures that MenuItems flex so that they all have the same height if any wrap their content to new
lines at smaller screen resolutions. Ensures no additional padding is set on MenuItem children of
MenuFullscreen.

image

BREAKING CHANGE: Menu no longer supports height, minHeight, maxHeight, size,
overflowY and display props. MenuItem no longer supports height, minHeight,
maxHeight, size, verticalAlign, overflow, overflowY, overflowX and
display props.

Adds invariant to ensure that the component does not render when children are passed but submenu
is an empty string

Current behaviour

MenuItems do not wrap/flex uniformly
When submenu is an empty string the children are still rendered but within the parent MenuItem

Checklist

  • Commits follow our style guide
  • Related issues linked in commit messages if required
  • Screenshots are included in the PR if useful
  • Unit tests added or updated if required
  • Playwright automation tests added or updated if required
  • Storybook added or updated if required
  • Typescript d.ts file added or updated if required

QA

  • Tested in provided StackBlitz sandbox/Storybook
  • Add new Playwright test coverage if required
  • Carbon implementation matches Design System/designs
  • UI Tests GitHub check reviewed if required

Additional context

Testing instructions

  • menu-test--when-menu-items-wrap story added for testing when items wrap
  • Worth doing brush test of all other stories to ensure no regressions
  • To test issue 7001 add the following code into any menu test story and confirm the invariant has fired:
<Menu menuType="black">
      <MenuItem href="#">Menu Item One</MenuItem>
      <MenuItem onClick={() => {}} submenu="">
        <MenuItem
          href="#"
          onClick={() => {
            alert("clicked");
          }}
          variant="alternate"
        >
          Link
        </MenuItem>
        <MenuItem href="#">Submenu Item Two</MenuItem>
      </MenuItem>
    </Menu>

@edleeks87 edleeks87 self-assigned this Oct 14, 2024
@edleeks87 edleeks87 marked this pull request as ready for review October 14, 2024 11:12
@edleeks87 edleeks87 requested review from a team as code owners October 14, 2024 11:12
@edleeks87 edleeks87 marked this pull request as draft October 14, 2024 12:13
@Parsium Parsium self-requested a review October 15, 2024 11:59
@Parsium

This comment was marked as outdated.

@DipperTheDan DipperTheDan self-requested a review October 15, 2024 15:06
Copy link
Contributor

@DipperTheDan DipperTheDan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

**issue: ** Seems if a MenuItem has a submenu and a href, the styling is a bit odd. These screenshots are from the Submenu Options story

Screenshot 2024-10-15 at 16 51 47

Screenshot 2024-10-15 at 16 52 00

@edleeks87
Copy link
Contributor Author

@edleeks87 I noticed the commit 831959d seems to reference the wrong issue. Would you be able to update this? :)

thanks I've made a typo there I'll address it

DipperTheDan
DipperTheDan previously approved these changes Oct 16, 2024
Parsium
Parsium previously approved these changes Oct 16, 2024
@Parsium Parsium marked this pull request as ready for review October 16, 2024 12:12
…ht if any wrap to new lines

Ensures that `MenuItem`s flex so that they all have the same height if any wrap their content to new
lines at smaller screen resolutions. Ensures no additional padding is set on `MenuItem` children of
`MenuFullscreen`.

BREAKING CHANGE: `Menu` no longer supports `height`, `minHeight`, `maxHeight`, `size`,
`overflowY` and `display` props. `MenuItem` no longer supports `height`, `minHeight`,
`maxHeight`, `size`, `verticalAlign`, `overflow`, `overflowY`, `overflowX` and
`display` props.

fix #6934, fix #7000
…` is an empty string

Adds invariant to ensure that the component does not render when `children` are passed but `submenu`
is an empty string

fix #7010
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants